Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added delete_tileset manage command and tileset API route #79

Closed
wants to merge 6 commits into from
Closed

Added delete_tileset manage command and tileset API route #79

wants to merge 6 commits into from

Conversation

alexpreynolds
Copy link
Contributor

@alexpreynolds alexpreynolds commented Nov 13, 2018

Here is a demonstration tileset:

$ python manage.py list_tilesets
tileset: Tileset [name: LN43287.75_20.normalized.GRCh38_no_alts.bw.tileset] [ft: hitile] [uuid: AIVpsJYwSemD8FVPBv6vrw]
$ ls -al higlass-server/media/uploads/
total 207764
drwxrwxr-x 2 ubuntu ubuntu      4096 Nov 13 06:15 ./
drwxrwxr-x 3 ubuntu ubuntu      4096 Nov 13 06:15 ../
-rw-rw-r-- 1 ubuntu ubuntu 212742028 Nov 13 06:15 LN43287.75_20.normalized.GRCh38_no_alts.bw.tileset

Here is an example showing the use of the manage.py delete_tileset command to remove this tileset file and database record:

$ python manage.py delete_tileset --uuid=AIVpsJYwSemD8FVPBv6vrw
$ python manage.py list_tilesets
$ ll higlass-server/media/uploads/
total 8
drwxrwxr-x 2 ubuntu ubuntu 4096 Nov 13 06:52 ./
drwxrwxr-x 3 ubuntu ubuntu 4096 Nov 13 06:15 ../

The API call uses the DELETE method and the IsAuthenticated permission class decorator, which requires authentication. Here is an example of an (unauthenticated) API call:

$ python manage.py list_tilesets
tileset: Tileset [name: LN43287.75_20.normalized.GRCh38_no_alts.bw.tileset] [ft: hitile] [uuid: MSLMji4TSVKynUGNui-5cQ]
...
$ curl -X "DELETE" http://localhost:8000/api/v1/tileset/?d=MSLMji4TSVKynUGNui-5cQ
{"uuid": "MSLMji4TSVKynUGNui-5cQ"}

The API returns 40x errors if the underlying instance or file is not found or cannot be removed.

For debugging purposes, I modified the Tileset model __str__ function to return the tileset's uuid along with its name and filetype properties.

@alexpreynolds
Copy link
Contributor Author

alexpreynolds commented Nov 13, 2018

I added a management command and API route to modify a tileset's properties by uuid. Currently, this only modifies the name field (if specified), but this could be expanded to modify other tileset properties with additional logic.

Here is an example management command:

$ python manage.py modify_tileset --uuid="Zfg_ZtU0TZeJYRoRQwRMFQ" --name="bar"

And here is an example API call:

$ curl -H "Accept: application/json" -X "POST" -d '{"uuid":"BAeHd65pSCmI8Cs-xS5ACw", "name":"foo"}' http://localhost:8000/api/v1/tileset/

@pkerpedjiev
Copy link
Member

This is great! Thanks Alex.

A few things before merging:

  1. As mentioned in the inline comments, could you check if the DELETE endpoint doesn't already exist in TilesetsViewSet?
  2. The documentation for higlass_server is here: https://github.com/higlass/higlass/blob/develop/docs/higlass_server.rst. Would you mind adding a few lines describing the new API endpoints?
  3. It looks like you've tested this quite extensively. To make sure we don't introduce regressions, could you just copy those manual tests to our automated test section here: https://github.com/higlass/higlass-server/blob/master/higlass_server/tests.py?

Thanks!

@@ -492,6 +494,95 @@ def tiles(request):
)

return JsonResponse(tiles_to_return, safe=False)


@api_view(['POST', 'DELETE'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check if the curl -X 'DELETE' server:port/api/v1/tilesets/[uuid]/ endpoint already exists? There's already a tilesets viewset that should provide, GET, POST, UPDATE, DELETE endpoints here:

class TilesetsViewSet(viewsets.ModelViewSet):

I know the POST endpoint works because I've been using it to upload datasets to higlass.io. Take a look here: http://docs-dev.higlass.io/data_preparation.html#importing-into-higlass

If possible, I'd prefer to avoid duplicating API endpoints.

@@ -55,6 +55,34 @@ curl http://localhost:8000/api/v1/tileset_info/?d=hitile-demo
curl http://localhost:8000/api/v1/tiles/?d=hitile-demo.0.0.0
```

## Preparing files for ingest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the datatype specific documentation is in the main documentation repository: http://docs-dev.higlass.io/data_preparation.html

The code for that can be found here: https://github.com/higlass/higlass/tree/develop/docs

@@ -1,8 +1,8 @@
Cython==0.25.2
-e git+https://github.com/mirnylab/cooler.git@master#egg=cooler
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's in the master branch that isn't in v0.7.10?

@alexpreynolds
Copy link
Contributor Author

Sorry. I'm going to close this PR and start over.

@pkerpedjiev
Copy link
Member

No, it's my fault that we don't have better documentation about the API endpoints. Ideally the docs would have a section on the REST API. Without looking at the code it's nearly impossible to figure out what endpoints exist and how to access them.

All this is to say, your PR is really appreciated and hopefully it's not too much work to fit it into TilesetViewSet.

@alexpreynolds
Copy link
Contributor Author

I neglected to sync my fork with upstream, so starting over was necessary, anyway.

Looks like the built-in DELETE does remove the database entry, but it does not remove the tileset file from the media/uploads directory. I'll look at overriding the behavior of the destroy() method to take care of that, so that it would behave the same as the management command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants